-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[IR] Remove size argument from lifetime intrinsics #150248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
a56aec7
to
ef647f8
Compare
Split out from llvm#150248: Since llvm#150944 the size passed to lifetime.start/end is considered meaningless. The lifetime always applies to the whole alloca. This adjusts MemoryLocation to determine the MemoryLocation size from the alloca size, instead of using the argument.
Split out from llvm#150248: Use the size of the alloca instead of the size passed to the lifetime intrinsic. As a bonus, this handles dynamic allocas correctly (see the added test) instead of doing a memset with size -1...
Split out from llvm#150248: Use the size of the alloca instead of the size passed to the lifetime intrinsic. As a bonus, this handles dynamic allocas correctly (see the added test) instead of doing a memset with size -1...
I think this is now ready for review |
Split out from #150248: Use the size of the alloca instead of the size passed to the lifetime intrinsic. As a bonus, this handles dynamic allocas correctly (see the added test) instead of doing a memset with size -1...
…154) Split out from llvm/llvm-project#150248: Use the size of the alloca instead of the size passed to the lifetime intrinsic. As a bonus, this handles dynamic allocas correctly (see the added test) instead of doing a memset with size -1...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming there aren't any non-trivial changes to the regression tests. (I looked at the code changes, but I skimmed the test changes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG
Now that llvm#149310 has restricted lifetime intrinsics to only work on allocas, we can also drop the explicit size argument. Instead, the size is implied by the alloca. This removes the ability to only mark a prefix of an alloca alive/dead. We never used that capability, so we should remove the need to handle that possibility everywhere (many key places, including stack coloring, did not actually respect this).
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/25074 Here is the relevant piece of the build log for the reference
|
In llvm/llvm-project#150248 LLVM removed the size parameter from the lifetime format. Tolerate not having that size parameter.
In llvm/llvm-project#150248 LLVM removed the size parameter from the lifetime format. Tolerate not having that size parameter.
In llvm/llvm-project#150248 LLVM removed the size parameter from the lifetime format. Tolerate not having that size parameter.
In llvm/llvm-project#150248 LLVM removed the size parameter from the lifetime format. Tolerate not having that size parameter.
Now that #149310 has restricted lifetime intrinsics to only work on allocas, we can also drop the explicit size argument. Instead, the size is implied by the alloca.
This removes the ability to only mark a prefix of an alloca alive/dead. We never used that capability, so we should remove the need to handle that possibility everywhere (though many key places, including stack coloring, did not actually respect this).